Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't send empty arguments to gcloud in the cleanup script #591

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

SirGitsalot
Copy link
Collaborator

SUMMARY

When the scripts/cleanup-project.sh calls cleanup_resource() with an empty value for either $extra_list_arg or $extra_delete_arg it will in turn call gcloud with the empty argument, which gets misinterpreted.

For instance, the script cleans up target http proxies with:

cleanup_resource "compute" "target-http-proxies" "" "--global"

The empty list argument is quoted in the call to gcloud (as it should be!), which means that the arguments that gcloud processes are ['gcloud', 'compute', 'target-http-proxies', 'list', '--project="blahblah"', '--format="csv[no-heading](name)"', '']

That last empty string argument is the reason for these log messages:

WARNING: Argument `NAME` is deprecated. Use `--filter="name=( 'NAME' ... )"` instead.

And it's the reason that the script ultimately fails, as no resources are listed thus no resources are deleted.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

Copy link
Collaborator

@hao-nan-li hao-nan-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on the empty arguments!

Looks like the integration test failed due to

TASK [gcp_sql_database : create a database that already exists] ****************
fatal: [testhost]: FAILED! => "changed": false, "msg": "SQL objects can't be updated to ensure data safety"

PLAY RECAP *********************************************************************
testhost : ok=8 changed=2 unreachable=0 failed=1 skipped=0 rescued=0 ignored=0

And it seems unrelated to the change. Perhaps some cleanup is needed here on the sql DB resource.

@SirGitsalot
Copy link
Collaborator Author

And it seems unrelated to the change.

Yeah, the integration weren't running before so this test may have been broken for a while (and the integration tests run sequentially and stop at the first failing test, so there may be other broken tests after this one). We should target getting them all back and running reliably. Probably not a good candidate task for this PR, though :-)

@SirGitsalot SirGitsalot merged commit 611e6d9 into master Sep 5, 2023
2 of 4 checks passed
@SirGitsalot SirGitsalot deleted the sirgitsalot-empty-cleanup-args branch September 5, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants